Charge value-graph walks so BCS/resource deserialize cannot amplify for free - #428
Open
sausagee wants to merge 3 commits into
Open
Charge value-graph walks so BCS/resource deserialize cannot amplify for free#428sausagee wants to merge 3 commits into
sausagee wants to merge 3 commits into
Conversation
BCS and resource loads could materialize a large Move value graph while only paying for blob bytes. Bill a compare-class walk (3x cmp::compare) behind MeterValueNodesOnDeserialize so a compact encoding cannot amplify for free. Cache hits stay unbilled; arithmetic saturates. Co-authored-by: Young Yang Liauw <sausagee@users.noreply.github.com>
The single nested struct literal was deep enough to overflow the Move compiler stack while publishing the e2e package. Co-authored-by: Young Yang Liauw <sausagee@users.noreply.github.com>
value is already a &Value after the deserialize-side walk charge. Co-authored-by: Young Yang Liauw <sausagee@users.noreply.github.com>
sausagee
marked this pull request as ready for review
September 6, 2026 05:29
sausagee
requested review from
0xIcarus,
Primata,
fEst1ck,
ganymedio,
musitdev and
seanyoung
as code owners
September 6, 2026 05:29
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Clean-room rewrite (new author, new names, new control flow). This is not a cherry-pick, patch port, or mechanical transcription of any aptos-labs change. Intent only: compact BCS / resource bytes can expand into a huge Move value-node graph while paying only for blob length.
Invariant: materializing or walking a Move value must not be free relative to that value’s abstract node size. A compact encoding that expands into many nodes must cost more once the timed flag is on. Arithmetic saturates so a huge graph over-charges rather than wrapping cheap.
MeterValueNodesOnDeserializegates both deserialize and serialize-side walks (serialize had the same hole: output-byte gas alone underprices a wide graph with a tiny encoding).Pricing is 3× m1
cmp::compare(base = 1101,per_abs_val_unit = 42), implemented asvalue_graph_walk_cost— not on-chain params yet.StandardGasMeter::newandmake_prod_gas_metersignatures are unchanged; the VM opts in viaenable_value_graph_load_billing.Charge sites (cache miss only where a session/table cache exists):
charge_load_resource)util::from_bytesexists_atto_bytes/serialized_sizeTESTING activation is 1970-01-01 03:00 UTC so one
new_epoch()(2h) leaves the flag off. Movement dates precede the 2025-08-11 catch-all so the flag is not live immediately.How Has This Been Tested?
All of the following passed locally:
cargo test -p aptos-gas-schedule --lib— includesvalue_graph(base cost, linear growth, saturate atu64::MAX)cargo test -p aptos-types --lib -- value_graph_flag— genesis / 2h off, 4h on; Movement 2026-09-06 still offcargo test -p aptos-gas-meter --lib— walk unbilled when flag off; billed size matches formula; missing resource = 0 executioncargo test -p aptos-framework -p aptos-native-interface -p aptos-table-natives -p aptos-move-stdlib -p aptos-memory-usage-tracker -p aptos-gas-profiling --libcargo test -p e2e-move-tests --lib -- value_graph— 7 tests: bushyCell(W1–W7 + 4-field Leaf).unpack_loop/serialize_loop/peek/peek_shelf. Enable via twonew_epoch(). Assertgas_on > gas_off. Large values still succeed (charged, not rejected).-D warningswith repo allow-list) on the touched gas / native / types crates.Key Areas to Review
(_, MOVEMAINNET | MOVETESTNET).loaded.is_some()/num_bytes.is_some()).exists_atkeep layout params and session cache borrows disjoint.unsafe. SaturatingInternalGasadd/mul. Charge before/with materialization where possible (to_bytesbefore serialize;from_bytesafter deserialize because abstract size is only known then).Type of Change
Which Components or Systems Does This Change Impact?
Security (
RUST_SECURE_CODING.md/SECURITY.md)Reviewed new code against the repo guidelines:
unsafe.GasQuantityadd/mul already saturate; walk cost uses those ops so a huge graph cannot wrap to a cheap bill.aptos-gas-algebraontable-natives).Result/?; no unwrap on charge paths.Aikido CLI is not available in this environment; the review above is the scan of the new code. Please run the project Aikido workflow on the PR if that is part of CI.
Checklist
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.